Skip to content

feat(sandbox-manager): add network interface binding and reliable peer discovery - #920

Merged
furykerry merged 8 commits into
openkruise:masterfrom
AiRanthem:feature/network-interface-peer-discovery-260821
Sep 10, 2026
Merged

furykerry merged 8 commits into
openkruise:masterfrom
AiRanthem:feature/network-interface-peer-discovery-260821

Conversation

@AiRanthem

@AiRanthem AiRanthem commented Sep 2, 2026 •

Copy link
Copy Markdown
Member

Ⅰ. Describe what this PR does

This PR adds an optional --network-interface flag for Sandbox Manager and uses its validated global-unicast address, preferring IPv4 over IPv6, consistently for the control API, peer route listener, memberlist bind/advertise address, and non-Sandbox proxy return path.

It also makes peer discovery resilient to startup ordering and temporary Kubernetes API failures:

  • validates the namespace, label selector, REST configuration, and selected network interface at startup;
  • lists selector-matching peer Pods with bounded retries and attempts every trusted seed in stable order during each cycle;
  • validates, deduplicates, and stably orders trusted seed addresses;
  • starts required listeners synchronously so bind failures reach the caller;
  • serializes Start/Stop and gives one lifecycle owner responsibility for Leave and Shutdown;
  • preserves upstream traffic-token and dedicated observability-listener behavior.

The design and lifecycle contract are documented in the accompanying proposal.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how to verify it

  • go test -mod=mod ./pkg/peers ./pkg/proxy ./pkg/sandbox-gateway/server ./pkg/sandbox-manager ./pkg/servers/e2b ./pkg/utils/network -count=1
  • go build -mod=mod ./cmd/sandbox-manager
  • git diff --check upstream/master...HEAD

Ⅳ. Special notes for reviews

  • An empty --network-interface preserves the existing non-hosted behavior.
  • A configured interface must be up and have exactly one global-unicast address in the preferred available family: IPv4 if any global-unicast IPv4 address exists, otherwise IPv6. Startup fails instead of falling back to another interface.
  • The ext-proc listener, metrics, pprof, and other observability listeners remain outside the selected user-network address contract.
  • Peer discovery attempts every trusted seed in a cycle, even after a successful Join. After the cycle completes, it stops Kubernetes listing and retrying if any Join succeeded; memberlist handles ongoing membership afterward.
  • An in-flight memberlist Join is bounded by existing memberlist network deadlines because the upstream Join API has no context parameter.

…r discovery

- Introduce optional --network-interface flag to bind Sandbox Manager to one user network IPv4 address
- Validate interface existence, status, and single global unicast IPv4 address on startup; fail if invalid
- Use resolved address uniformly for control API, peer route service, memberlist binding/advertisement, and non-Sandbox proxy return
- Preserve existing non-hosted behavior when flag is empty, using POD_IP or first non-loopback address
- Reuse existing controller-runtime cache scoped by namespace and peer label selector for peer discovery; avoid separate client/cache
- Establish trusted seed addresses from cached Pods with filtering on memberlist-url annotation and duplicates
- Implement background, non-blocking lifecycle for peer discovery and joining with retries and network deadline aware shutdown
- Enforce one lifecycle owner for graceful Leave and Shutdown on stop requests; prevent concurrent cleanup races
- Ensure startup fails on invalid network scope/configuration before serving requests; seed discovery not a startup blocker
- Maintain compatibility by adding only CLI flag without CRD, HTTP model, or protocol changes and preserving existing user cluster config
- Document risks including resource use for Pod informer, delayed join cancellation, stale member presence on forced termination, and address change requiring restart

Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>
… discovery

- Add --network-interface flag to specify network interface for user-cluster traffic
- Resolve and validate the network interface IP address at startup
- Introduce --disable-envoy-ext-proc flag to disable Envoy ext-proc gRPC listener
- Pass resolved bind address and ext-proc disable flag to SandboxManagerOptions
- Update sandbox controller Run method to accept stop channel for graceful shutdown
- Refactor peer discovery to use a live, uncached Kubernetes client with namespace and label selector
- Implement bounded Pod listing with retries until joining memberlist succeeds
- Ensure peer discovery stops retrying after successful join to avoid cache overhead
- Add lifecycle management for peer discovery and memberlist join with once-only stop semantics
- Enhance memberlist peers start/stop logic with context handling and IP binding
- Add comprehensive unit tests covering start/stop, join lifecycle, peer leave, seed address trust,
  retry behavior, and namespace/label filtering in peer discovery
- Update proposal documents to reflect design and implementation details for peer discovery and
  network interface usage

Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>
- Add detailed join seed recording in fakeMemberlistHandle for better verification
- Refactor peerlist tests to assert joined seeds with namespace and label selector filtering
- Simplify and enhance synchronization in join retry and join order tests using atomic counters
- Add startBlockedJoin helper to test Join call blocking and graceful Stop handling
- Replace deprecated test patterns with improved concurrency control and clearer assertions

test(ext_proc): consolidate server run lifecycle tests

- Replace separate tests with table-driven TestServerRunLifecycle covering
  - synchronous bind failures for grpc listener
  - normal run and stop releasing both listeners
  - skipping grpc listener when ext-proc disabled
  - stopping before run prevents late start
- Verify listener release after stop in all scenarios

test(sandbox-manager): ensure cache start before peers and pass bind address

- Merge tests to check cache starts before peers and peers receive bind address and port
- Remove redundant assertions and simplify setup

test(servers/e2b): add newStartupController helper and improve startup signal handling tests

- Extract newStartupController to enable testing controller start behavior with custom run functions
- Adjust TestControllerSignalDuringStartupExitsCleanly to use new helper and verify expected zero exit semantics
- Update TestControllerRunPropagatesStartupFailure to use newStartupController and check synchronous startup error surfacing

test(utils/network): combine ResolveNetworkInterfaceAddress tests

- Unify empty and missing interface tests into a table-driven test for ResolveNetworkInterfaceAddress
- Confirm error messages and returned addresses for different interface names correctly

Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>
@kruise-bot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign furykerry for approval by writing /assign @furykerry in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov

codecov Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.29%. Comparing base (a894e17) to head (5d1fedc).
⚠️ Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
pkg/peers/memberlist.go 90.07% 7 Missing and 6 partials ⚠️
pkg/sandbox-manager/core.go 60.00% 7 Missing and 3 partials ⚠️
pkg/proxy/server.go 72.72% 5 Missing and 4 partials ⚠️
pkg/utils/network/network.go 85.00% 6 Missing ⚠️
pkg/servers/e2b/core.go 92.00% 2 Missing and 2 partials ⚠️
pkg/sandbox-gateway/server/server.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #920      +/-   ##
==========================================
+ Coverage   83.08%   83.29%   +0.20%     
==========================================
  Files         257      259       +2     
  Lines       22536    22715     +179     
==========================================
+ Hits        18724    18920     +196     
+ Misses       3097     3060      -37     
- Partials      715      735      +20     
Flag Coverage Δ
unittests 83.29% <85.71%> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Use atomic bool flag to mark memberlist start completion
- Gate GetPeers, GetAllMembers, LocalAddr, LocalPort on started flag instead of list nil-check
- Add test to simulate concurrent getters racing with Start
- Publish list before the started flag is set, so getters never observe a nil list
- Prevent nil dereference and stale reads on memberlist data before start completion
- Keep a single English proposal (drop the CN copy, rename the EN file) and align the --network-interface flag description

Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>
…proposal

Address review findings on the network-interface peer discovery branch.

Proposal:
- Document the startup crash model: a termination signal during startup
  exits without graceful cleanup, while a signal observed together with a
  completed startup runs the graceful Stop path.
- Document Start exactly once and Stop at most once for the peer
  lifecycle instead of idempotent repeated Stop.
- Bring --disable-envoy-ext-proc into scope with its contract.
- Exclude terminal-phase Pods from the seed set; describe single-replica
  listing behavior, the Gateway PEER_NAMESPACE/PEER_LABEL_SELECTOR
  requirement, and where the peer client and interface resolution live.

Code:
- peers: drop the speculative Stop-before-Start guard and reject a second
  Start instead; share the lifecycle tail between Start and tests; wait a
  full retry interval after each attempt; log an empty seed set; always
  advertise the bound address; skip Succeeded/Failed Pods.
- proxy: drop the stopped flag and the redundant listener fields; the
  servers own their listeners.
- sandbox-manager: start the proxy before memberlist so the peer route
  listener serves before this replica is advertised; replace the elector
  runOnce with an atomic claim so Stop keeps honoring its context.
- e2b: prefer an already-available startup result when a signal arrives
  so a fully started controller shuts down gracefully.
- Makefile: run test packages serially while several test binaries bind
  the fixed route-refresh and ext-proc ports.

Tests: table-driven seed selection, shared lifecycle wiring, startup
ordering against an occupied port, awaitStartup both-ready rows, and the
controller startup tests moved to run_test.go.

Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>
@AiRanthem
AiRanthem force-pushed the feature/network-interface-peer-discovery-260821 branch from 534bed2 to 6921fc6 Compare September 4, 2026 11:03
Comment thread pkg/utils/network/network.go Outdated
Comment thread pkg/peers/memberlist.go
AiRanthem and others added 3 commits September 8, 2026 23:28
IPv6 is out of this change's scope. Keep waiting on ctx after a successful join so Leave/Shutdown still run from the lifecycle owner.

Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>
Interface resolution now prefers the single global-unicast IPv4 address and falls back to IPv6 when no IPv4 qualifies. Seed derivation accepts IPv4 and IPv6 pod IPs, bracketing IPv6 host:port pairs. tryJoin joins every seed each cycle instead of stopping at the first success, keeping per-seed Join calls for attributable failures and cancellation between dials. Update the proposal to match the dual-stack contract and join-all lifecycle.

Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>
…addresses

Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>
@furykerry
furykerry merged commit 8765d81 into openkruise:master Sep 10, 2026
29 of 30 checks passed
furykerry added a commit that referenced this pull request Sep 11, 2026
* fix(security): address CodeQL path-injection and log-injection alerts (#928)

* fix(security): address CodeQL path-injection and log-injection alerts

Add path validation barriers and log sanitization to resolve 43 CodeQL
security alerts:
- 31 go/path-injection alerts (CWE-22/23/36/73/99)
- 12 go/log-injection alerts (CWE-117)

Path validation:
- Add validateSafePath helper that rejects empty paths and ".." segments
- Apply validation before filesystem operations in symlink creation,
  atomic writer, cert writer, and mount finder
- Validate symlink targets and data-directory symlink reads

Log sanitization:
- Add sanitizeLogValue helper to strip newlines and carriage returns
- Apply to all log statements using user-controlled or filesystem paths
- Prevents log injection through crafted file names or error messages

Affected packages:
- cmd/sandbox-gateway-cert-init
- pkg/agent-runtime/storage-cli
- pkg/agent-runtime/storage-cli/link
- pkg/agent-runtime/storage-cli/mountfinder
- pkg/utils/webhookutils/writer
- pkg/utils/webhookutils/writer/atomic

Signed-off-by: 守辰 <shouchen@users.noreply.github.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

* refactor(security): centralize path and log sanitizers

Share the duplicated validation and log-value sanitization helpers so the
security barriers have one tested implementation to maintain.

Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

---------

Signed-off-by: 守辰 <shouchen@users.noreply.github.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>
(cherry picked from commit 871ff6e)

* docs: add v0.6.0-alpha1 release notes (#932)

Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>
(cherry picked from commit 27c7201)

* fix(sandbox): propagate startup failures to wait-ready and ScalingLimited (#936)

(cherry picked from commit fd02f31)

* feat(poolautoscaler): enable feature gate by default (#938)

Signed-off-by: 少师 <zengyuwei.zyw@alibaba-inc.com>
Co-authored-by: 少师 <zengyuwei.zyw@alibaba-inc.com>
(cherry picked from commit 0ba722a)

* refactor(controller): generalize Pod status synchronization (#939)

(cherry picked from commit 1781b7c)

* feat(sandbox-manager): load secret config at startup and add startup hook (#857)

(cherry picked from commit 0e46dfa)

* fix(sandbox): classify unschedulable startup failures (#942)

* fix(sandbox): classify unschedulable startup failures

Signed-off-by: 少师 <zengyuwei.zyw@alibaba-inc.com>

* fix(sandbox): report startup failure reason before pod IP check in claim diagnostics

Unschedulable sandboxes have no node assignment and therefore no pod IP,
so the pod IP check masked the startup failure reason in wait timeout
diagnostics. Check startup failure reasons first so the scheduler message
is reported, and add a test covering an empty pod IP with an Unschedulable
ready condition.

Signed-off-by: 少师 <zengyuwei.zyw@alibaba-inc.com>

---------

Signed-off-by: 少师 <zengyuwei.zyw@alibaba-inc.com>
Co-authored-by: 少师 <zengyuwei.zyw@alibaba-inc.com>
(cherry picked from commit 18d90c1)

* fix: unbreak go vet and make build on master (#940)

The csi spec bump in #876 (v1.9.0 to v1.13.0) gave
csi.NodePublishVolumeRequest a protoimpl.MessageState, which embeds a
sync.Mutex. The storage-cli passes that struct by value throughout, so
go vet started reporting 18 copylocks findings and exiting 1.

Makefile declares vet as a prerequisite of both build and test-e2e, so
neither ran. Take the request by pointer instead, which is the normal
way to handle a protobuf message and removes a real struct copy from the
mount path.

make build still produced no binary after that, for a second and
unrelated reason: the target compiled a single file rather than the
package, so executeCABindings in ca_binding.go was invisible to it.
Build the package, matching what the okactl target two lines below
already does.

go vet ./... is now clean and make build produces
bin/agent-sandbox-controller.

Fixes #905

Signed-off-by: Om <omlahore47@gmail.com>
(cherry picked from commit cb37dce)

* feat(sandbox-manager): add network interface binding and reliable peer discovery (#920)

(cherry picked from commit 8765d81)

* fix(network): format IPv6 upstream addresses (#901)

(cherry picked from commit 6971312)

* build(deps): bump github/codeql-action/upload-sarif (#949)

Bumps [github/codeql-action/upload-sarif](https://github.com/github/codeql-action) from 4.37.8 to 4.37.9.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@db488dd...cdf488f)

---
updated-dependencies:
- dependency-name: github/codeql-action/upload-sarif
  dependency-version: 4.37.9
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
(cherry picked from commit 5735cf9)

* chore(skill): drop Deployment from sync-charts manifests drift check (#944)

* chore(skill): drop Deployment from sync-charts manifests drift check

Deployment manifests are no longer synchronized by the manifests
drift checker. The comparison now covers only Service, ConfigMap,
Ingress, and Secret across the controller, manager, and gateway
components. This removes Deployment-only comparison functions,
simplifies MANIFEST_SPEC to seven non-Deployment entries, and
updates the test suite and SKILL.md accordingly.

Signed-off-by: 守辰 <shouchen@example.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

* docs(skill): codify sync policy for manifests drift fixes

Document the three-rule sync policy applied when splicing DRIFT
findings into chart templates: never modify resource metadata,
append only to spec (and to data/stringData on ConfigMap/Secret)
without changing existing field values, and never replace a
Helm {{ ... }} template expression with a concrete source value.
Tests assert the three rules appear in SKILL.md.

Signed-off-by: 守辰 <shouchen@example.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

* docs(skill): drop append-only constraint on spec sync policy

The spec append-only rule is removed from the manifests sync
policy. The remaining rules are: do not modify resource metadata,
and never replace a Helm {{ ... }} template expression with a
concrete source value on spec, data, or stringData fields.

Signed-off-by: 守辰 <shouchen@example.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

* feat(skill): classify templated-field drift and index-based port compare

The manifests drift checker now reads the raw chart template file and
reclassifies a value-difference DRIFT as TEMPLATED when the affected
field is rendered through a {{ ... }} expression or a {{- range }}
block, guiding the fix toward updating the values.yaml default instead
of hardcoding a literal. Service ports are compared by index so
source order matters and chart-only ports are reported by position.
The sync policy is restated as three rules: preserve chart-managed
metadata while adding source-defined labels/annotations, keep
templates and fix values-driven drift via values.yaml defaults, and
match source order for indexed lists. Verification now requires a
manual diff review for template regressions, because a hardcoded
value that matches the source renders identically and cannot be
detected by the checker.

Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

---------

Signed-off-by: 守辰 <shouchen@example.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>
(cherry picked from commit 51149a4)

* Tracing user operation (#950)

* feat(tracing): emit trace ID as the first JSON log field

Install a trace-first JSON zap encoder for the controller so each log line
carries the trace ID as its leading field, keeping log collectors that index
the first field stable.

Co-authored-by: WWKKAA <1938897817@qq.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>

* feat(tracing): record Sandbox conditions on controller spans

Attach each Sandbox condition as a "Status:Reason" span attribute on both the
Reconcile span (before) and the updateSandboxStatus span (after), so a single
trace shows how a Reconcile iteration drove the condition transition.

Co-authored-by: WWKKAA <1938897817@qq.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>

* feat(tracing): propagate user operations across components

Carry the user-facing operation (create/pause/resume/kill) as OTel baggage from
the API entry point through to controller Reconcile via a CR annotation, and
surface it as the traceOperation log field so cross-component logs can be
filtered by operation.

Co-authored-by: WWKKAA <1938897817@qq.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>

* fix(tracing): emit stdout spans as single-line JSON

Drop pretty-print from the std-mode stdout exporter so newline-based log
collectors treat each span as one entry instead of splitting it into fragments.
File mode keeps pretty-print since it is read directly, not line-collected.

Co-authored-by: WWKKAA <1938897817@qq.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>

* fix(tracing): 在 connect 路由入口统一标记 resume

Signed-off-by: 马赫 <mahe@bupt.edu.cn>

* fix(e2e): 按 JSON 字段校验 tracing 日志

Co-authored-by: WWKKAA <1938897817@qq.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>

* fix(tracing): address encoder and baggage propagation edge cases

Document overridden zap encoder and time flags, handle empty JSON objects, and clear stale baggage before trace injection. Add regression coverage for encoder output and baggage replacement.

Co-authored-by: WWKKAA <1938897817@qq.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>

---------

Signed-off-by: 马赫 <mahe@bupt.edu.cn>
Co-authored-by: WWKKAA <1938897817@qq.com>
(cherry picked from commit 3036e82)

---------

Signed-off-by: 守辰 <shouchen@users.noreply.github.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>
Signed-off-by: 少师 <zengyuwei.zyw@alibaba-inc.com>
Signed-off-by: Om <omlahore47@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: 守辰 <shouchen@example.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>
Co-authored-by: ywExcellent <yuweizeng97@163.com>
Co-authored-by: 少师 <zengyuwei.zyw@alibaba-inc.com>
Co-authored-by: Ai Ranthem <zhongtianyun.zty@alibaba-inc.com>
Co-authored-by: Om Lahore <omlahore47@gmail.com>
Co-authored-by: Shirui Cheng <34178628+cyrilcsr@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: He Ma <71874811+Liquorice-Ma@users.noreply.github.com>
Co-authored-by: WWKKAA <1938897817@qq.com>
zhuangzhewei09 added a commit to zhuangzhewei09/agents that referenced this pull request Sep 20, 2026
The merged proposals in docs/proposals/ (e.g. openkruise#966 checkpoint, openkruise#920
peer-discovery, openkruise#895 scale-up coordination) carry metadata only in the
YAML frontmatter. Remove the redundant Markdown metadata table and move
its one increment, the tracking issue link, into frontmatter see-also
(precedent: 20260626-sandbox-auto-pause.md uses see-also for external
URLs).

Signed-off-by: zhuangzhewei09 <zhuangzhewei09@dingtalk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants